Skip to content

Add azd ai dataset extension for Foundry datasets - #9499

Open
Mohamed Hessien (m7md7sien) wants to merge 85 commits into
Azure:mainfrom
m7md7sien:feat/azure-ai-dataset
Open

Add azd ai dataset extension for Foundry datasets#9499
Mohamed Hessien (m7md7sien) wants to merge 85 commits into
Azure:mainfrom
m7md7sien:feat/azure-ai-dataset

Conversation

@m7md7sien

@m7md7sien Mohamed Hessien (m7md7sien) commented Aug 10, 2026

Copy link
Copy Markdown

Adds azd ai dataset ΓÇö a standalone extension for registering and versioning Foundry datasets.

Why its own extension

A dataset is a general Foundry asset. Fine-tuning and other scenarios need one as much as evaluation does, so the CRUD group does not belong inside the evaluation extension. This extension has its own Go module, lint workflow and release pipeline, and no dependency on azure.ai.evaluations.

The line drawn with the evaluation extension: CRUD moved, generation stayed. generate writes the datasets: entry into the evaluation configuration, so moving it would strand a config write across an extension boundary ΓÇö a generated dataset registered with the service and absent from configuration, which azd up would not reconcile. create/update/list/show/delete/versions need no configuration at all, which is why they could move.

Surface

azd ai dataset create <name>      register a dataset, publishing version 1
azd ai dataset update <name>      publish a new version
azd ai dataset list               list the project's datasets
azd ai dataset show <name>        show a version
azd ai dataset delete <name>      delete a version
azd ai dataset versions list <name>

Notable

  • Every user-facing string lives in one file (internal/messages/messages.go) so the whole voice of the CLI can be reviewed in one sitting. 69 messages.
  • TYPE, not FORMAT. The API accepts format on upload and never returns it, so the previous column was blank for every dataset ever listed.
  • Identity fixed. Carved from the evaluation extension, it had inherited its User-Agent (azd-ext-azure-ai-evaluations) and debug log name (azd-ai-eval-*.log), so a dataset request could not be told from an eval one in a service log or a local one.
  • Auth errors say what to do. A credential that cannot mint a token fails before any 401 and surfaced as a bare SDK error. It now suggests retrying first and logging in second ΓÇö observed failing transiently against a login that was valid.
  • A name the service will reject is refused locally. A dataset name with a space came back as a 400 wrapped in four levels of nested JSON; it is now checked against the accepted character set before the request.
  • A UTF-8 BOM no longer corrupts an upload. Windows editors and PowerShell write one by default, and it was being uploaded into the first row's first key rather than skipped.
  • Nil-guarded the endpoint resolution cascade and closed the azd client on the error paths that abandoned it.

Verified

go build, go vet -tags live,hero, go test ./... and gofmt clean. Exercised live against a Foundry project: create → 1.0, update → 2.0, versions list, show, list, and published to a local registry for a bug bash through real azd.

Not included, deliberately

No registry.json entry and no microsoft.foundry bundle dependency. Both need published artifacts; listing an unpublished extension in the bundle breaks azd extension install microsoft.foundry for everyone. Those land after the first release.

Open question for reviewers

azd ai dataset versions list <unknown-name> errors and exits 1 here, while the same command in the evaluation extension (#9500) lists nothing and exits 0. I tried making the eval side match this one and it broke two tests whose names are the argument against it ΓÇö "an unknown name lists nothing rather than failing" and "deleting an unregistered dataset is idempotent, not an error", the second because delete idempotence is verified by listing what is left.

A list is a filter rather than a lookup, so empty-and-successful is defensible and -o json callers range over the array instead of branching on an error. By that reasoning this extension's versions list is the one that should move. dataset show on an unknown name is not in question: that one is a lookup and should fail, as it does.

Draft ΓÇö not requesting reviewers yet.

Fixes #9550

Datasets are a general Foundry asset that fine-tuning and other scenarios
need as much as evaluation does, so the CRUD group ships as its own
extension rather than inside azure.ai.evaluations. Generation stays with
the evaluation extension, because it writes that extension's config file.

Standalone: no dependency on azure.ai.evaluations, its own go module,
lint workflow and release pipeline.
Adds internal/messages, holding the extension's errors, warnings and status lines behind one named function each, grouped under Datasets, Config, Output and Talking to the service. Wording is unchanged; the call sites now name the situation instead of spelling out the sentence. Cobra help, wire constants and table headers stay where they are.
Three things carried over from the extension this was carved out of. The
User-Agent announced azure-ai-evaluations and the debug log was written to
azd-ai-eval-<date>.log, so a dataset request could not be told from an eval
one in either a service log or a local one.

The FORMAT column was worse than wrong: format is a field the API accepts on
upload and never sends back, so the column was blank for every dataset ever
listed. It shows type, which the service does return.
Carved from the same code, so it had the same two: a nil Environment or
GetValue response panicking inside the endpoint cascade, and an azd client
abandoned when endpoint or credential setup failed.
Same credential failure and same 401/403 path as the eval extension, which
this was carved from.
Same intermittent credential failure as the eval extension.
Same three as the eval extension, which this was carved from.
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
7 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the customer-reported identify a customer issue label Aug 10, 2026
A BOM at the head of a .jsonl is uploaded as part of the first row's first
key, so every consumer of the dataset sees one malformed record -- and nothing
fails until something tries to read that row. Notepad, VS Code on Windows and
PowerShell all write one.

A file holding nothing but a BOM is still empty, and is still refused as such.
Found by bug-bashing the off-path cases.

A missing dataset reported "has no versions", which is not what happened -- a
dataset cannot exist with no versions, and the service answers an unknown name
with an empty list rather than a 404. Listing its versions was worse: it said
"No datasets found", as though the project had none at all. Both now say the
name is not a dataset and point at the listing, matching how the evaluations
extension already answers the same question.

A name with a space took a file upload and then a 400 carrying four levels of
nested JSON, with the sentence that matters at the bottom. The rule is simple
and documented, so it is checked before the round trip.
A mistyped path surfaced as 'GetFileAttributesEx <path>: The system cannot find the file specified.' The syscall name is noise to the person who mistyped it.
--from-file <file> resolved to the file's DIRECTORY, and the upload helper then took whichever .jsonl sorted first. Pointing at one dataset in a folder holding several registered a different one under that name. A test pinned the old behaviour: 'a file resolves to the directory the upload scans'.
The unit tests cover the client layer and the helpers; nothing exercised the command surface a user touches - flag parsing, exit codes, the rendered table, and whether -o json emits something a script can consume. 18 tests behind the live tag, gated on AZURE_AI_DATASET_E2E_LIVE, type-checked by ci-test.ps1 like the eval extension's suites.
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
7 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a standalone Foundry dataset extension with CRUD/versioning commands, API clients, tests, documentation, and release automation.

Changes:

  • Adds dataset creation, updates, listing, inspection, deletion, and version listing.
  • Adds endpoint resolution, validation, structured errors, and JSON/table output.
  • Adds unit/live tests plus build, lint, and release infrastructure.

Reviewed changes

Copilot reviewed 49 out of 50 changed files in this pull request and generated 19 comments.

Show a summary per file
File Description
eng/pipelines/release-ext-azure-ai-dataset.yml Adds extension release pipeline.
cli/azd/extensions/azure.ai.dataset/version.txt Sets initial beta version.
cli/azd/extensions/azure.ai.dataset/tests/cli/harness_test.go Adds live CLI harness.
cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go Tests dataset CLI workflows.
cli/azd/extensions/azure.ai.dataset/README.md Documents installation and commands.
cli/azd/extensions/azure.ai.dataset/main.go Adds extension entry point.
cli/azd/extensions/azure.ai.dataset/internal/version/version.go Defines build metadata.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/version_test.go Tests version handling.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/uri_test.go Tests URI resolution.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/upload_version_test.go Tests conflict-aware uploads.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go Implements dataset and storage operations.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations_wire_test.go Tests API request contracts.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go Defines API models and file handling.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/list.go Implements listing and version ordering.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/download_wire_test.go Tests download HTTP behavior.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/download_test.go Tests blob selection logic.
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/bom_test.go Tests BOM and file handling.
cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go Centralizes user-facing messages.
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/validator.go Validates Foundry endpoints.
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/types.go Defines endpoint-resolution types.
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/store.go Reads shared project context.
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go Implements endpoint precedence.
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver_test.go Tests endpoint resolution.
cli/azd/extensions/azure.ai.dataset/internal/exterrors/errors.go Adds structured error helpers.
cli/azd/extensions/azure.ai.dataset/internal/exterrors/codes.go Defines structured error codes.
cli/azd/extensions/azure.ai.dataset/internal/cmd/table_test.go Tests table rendering.
cli/azd/extensions/azure.ai.dataset/internal/cmd/surface_test.go Tests command surface and flags.
cli/azd/extensions/azure.ai.dataset/internal/cmd/root.go Builds the command tree.
cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go Implements JSON and text output.
cli/azd/extensions/azure.ai.dataset/internal/cmd/output_test.go Tests output formatting.
cli/azd/extensions/azure.ai.dataset/internal/cmd/names.go Validates dataset names.
cli/azd/extensions/azure.ai.dataset/internal/cmd/names_test.go Tests naming constraints.
cli/azd/extensions/azure.ai.dataset/internal/cmd/helpers_test.go Tests command helpers.
cli/azd/extensions/azure.ai.dataset/internal/cmd/debug.go Configures debug logging.
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go Implements dataset commands.
cli/azd/extensions/azure.ai.dataset/internal/cmd/context.go Creates command service context.
cli/azd/extensions/azure.ai.dataset/internal/cmd/artifacts.go Defines persistence helpers.
cli/azd/extensions/azure.ai.dataset/internal/cmd/apiversions.go Defines service API versions.
cli/azd/extensions/azure.ai.dataset/go.mod Defines the Go module.
cli/azd/extensions/azure.ai.dataset/extension.yaml Declares extension metadata.
cli/azd/extensions/azure.ai.dataset/cspell.yaml Configures spell checking.
cli/azd/extensions/azure.ai.dataset/ci-test.ps1 Adds CI test execution.
cli/azd/extensions/azure.ai.dataset/ci-build.ps1 Adds CI binary builds.
cli/azd/extensions/azure.ai.dataset/CHANGELOG.md Records the initial release.
cli/azd/extensions/azure.ai.dataset/build.sh Adds Unix cross-platform builds.
cli/azd/extensions/azure.ai.dataset/build.ps1 Adds Windows cross-platform builds.
cli/azd/extensions/azure.ai.dataset/.golangci.yaml Configures Go linting.
cli/azd/extensions/azure.ai.dataset/.gitignore Ignores generated artifacts.
.github/workflows/lint-ext-azure-ai-dataset.yml Adds GitHub lint workflow.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cli/azd/extensions/azure.ai.dataset/ci-build.ps1 Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/build.sh Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/build.ps1 Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go
Comment thread cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/internal/exterrors/errors.go Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/CHANGELOG.md Outdated
Two lines of ci-test.ps1 assumed PowerShell 6 or later. $IsWindows does
not exist on Windows PowerShell 5.1, so the .exe suffix was never
appended; and Join-Path there takes a single child path, so the
three-argument call failed outright. The script then found no gotestsum,
printed the fallback notice, and ran bare `go test` with no JUnit report
-- while still exiting 0, which is why it went unnoticed.
Copilot AI review requested due to automatic review settings August 10, 2026 05:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 73 out of 74 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:269

  • azd-code-reviewer: The promised “unknown name lists nothing” behavior is not implemented for the service's 404 response. ListDatasetVersions returns that 404 as an error (the API tests model this case), so this branch exits before renderDatasets can emit the empty array/message. Convert only IsNotFound(err) to an empty DatasetList and continue; keep propagating other failures.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:591

  • azd-code-reviewer: A malformed 200 response is currently converted into an empty container. readBlobPage then succeeds, and the caller reports that the dataset “holds no downloadable file,” hiding the actual storage/protocol failure. Return the XML decoding error from parseBlobPage and propagate it through readBlobPage instead of treating invalid XML as an empty listing.
    cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:51
  • azd-code-reviewer: Cleanup failures are silently ignored here and delete failures below are unchecked, so a green live suite can leave dataset versions in the shared Foundry project. Make the list decode and every delete required successes so cleanup failures are reported instead of leaking test state.
    cli/azd/extensions/azure.ai.dataset/README.md:22
  • This command as documented always fails because delete requires --version. Include the required flag so the command table contains a runnable invocation.
| `azd ai dataset delete <name>` | Delete a dataset version |

Comment thread cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go
An adversarial round over the newest code found three defects introduced by
the commits that were meant to fix things, and one that was already there.

Expanding an alias makes the entry longer than the `*name` it replaced, so the
line number rebased onto the file from the snippet landed further down than the
typo -- on a valid, unrelated entry, or past the end of the file. A confident
accusation against correct code is worse than no line at all, so where anything
expanded the entry itself is named. Aliases are also resolved once now rather
than twice: the second pass found nothing left and reported that nothing had
expanded.

Containment refused a symlink and let a Windows directory junction through. Go
reads a junction as an irregular file rather than a link, so EvalSymlinks
refuses the path while the OS walks the read straight through it -- and the
previous code read that refusal as "there is nothing here to follow". Only a
path that is genuinely absent is let past now. This is the cheaper of the two
escapes: mklink /J needs neither elevation nor Developer Mode.

`generate --wait` was parsed into a variable nobody read. `--wait=false` is a
legal spelling and asks for exactly what --no-wait asks for; accepted and
discarded, it made the command wait.

The run detail view drops a field with no value, which is right for a field
nobody needs and wrong for the one the command exists to report: status is
omitempty on the wire, and the row simply vanished. It says so instead.

Also, an entry that is itself an alias -- `- *base`, the most natural thing an
anchor is for -- was refused before the alias was looked at, and refused by
printing yaml's own enum: "must be a mapping, got 16". Both fixed. And the
dataset extension's error codes still carried the toolbox and skill vocabulary
they were copied from, 34 codes and 9 operation names for resources it has no
concept of, which the eval copy had already been cleaned of.

The merge-key test added last commit put its typo on the anchor rather than on
the entry that merges it, so decoding stopped before the merge was ever read.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 73 out of 74 changed files in this pull request and generated no new comments.

Suppressed comments (5)

cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:502

  • Use Go 1.26's errors.AsType here; this repository's modernization checks reject the older target-variable errors.As pattern.
    cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:29
  • azd-code-reviewer: Unknown output formats are silently treated as human output because the inherited flag is never constrained. For example, -o yaml exits successfully and prints a table, and -o none still prints output. Register per-command azdext.FlagOptions with the formats each command actually supports so invalid values fail before RunE.
	return strings.ToLower(v)

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/pages.go:61

  • azd-code-reviewer: Hitting a repeated link or the page cap returns a partial listing as a successful result. Callers such as LatestVersion and UploadNextVersion then treat that prefix as complete and can choose an already-used version; the direct os.Stderr warning also bypasses the command's injected writer. Return a truncation error (or explicit incomplete result) and let the command render it through its writer instead of continuing with partial data.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:207
  • azd-code-reviewer: Uploading loads the complete dataset into memory, converts it to a string, and later converts it back to []byte, so a large Foundry dataset requires several full-size allocations before the HTTP client can send anything. Stream validation and upload from the file (while handling the BOM on the reader) to keep memory bounded by row/buffer size.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:51
  • These duplicate fields leak the service's inconsistent spelling into the public -o json contract: a response containing data_uri is re-emitted as data_uri, while current responses emit dataUri. Scripts therefore need two schemas even though ResolvedBlobURI already normalizes them internally. Normalize during unmarshalling or map to a stable output DTO before encoding.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 73 out of 74 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:269

  • azd-code-reviewer: A 404 still exits non-zero here, despite this command's documented contract that an unknown name returns an empty successful list. The client and its tests explicitly treat a first-page 404 as the service's “dataset does not exist” response, so normalize that status to an empty DatasetList before rendering.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:335

  • azd-code-reviewer: An unknown dataset can be returned as a 404 by this API, but that path bypasses the concise DatasetNotFound handling below and exposes the wrapped service response instead. Translate IsNotFound(err) here so show has the same brief not-found behavior for both empty-list and 404 service variants.
				list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
				if err != nil {
					return messages.ResolvingLatestDatasetVersion(name, err)
				}

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:591

  • azd-code-reviewer: XML decoding failures are discarded, so malformed or truncated Storage responses become an empty successful listing and are later reported as “dataset holds no downloadable file.” Return the unmarshal error through readBlobPage so transport/protocol failures are not misclassified as valid empty datasets.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:51
  • azd-code-reviewer: The service spellings are modeled as separate exported JSON fields, so -o json emits dataUri for one backend response and data_uri for another (and likewise for blob/content URIs). That makes the scripting schema depend on the service payload; the CLI test already expects the stable dataUri name. Normalize these aliases during unmarshalling or render a dedicated output DTO with one canonical field set.
    cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:29
  • azd-code-reviewer: Unsupported --output values silently fall through to human output because none of the leaf commands registers SDK FlagOptions. Established extensions register AllowedValues: []string{"json", "table"} and a default (for example azure.ai.connections/internal/cmd/connection.go:97-99), which also keeps help and metadata accurate. Register those options on every runnable command instead of treating any non-JSON string as table output.
	v, err := cmd.Flags().GetString("output")
	if err != nil {
		return ""
	}
	return strings.ToLower(v)

cli/azd/extensions/azure.ai.dataset/README.md:22

  • This documented invocation always fails because delete requires --version. Include the required flag so users can run the example as written.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:369

  • The resolver newly reads both endpoint variables, but the environment-variable reference documents only FOUNDRY_PROJECT_ENDPOINT and describes it as agents-only; AZURE_AI_PROJECT_ENDPOINT is absent. Update cli/azd/docs/environment-variables.md in this PR so the repository's environment-variable source of truth matches this extension's fallback behavior.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 73 out of 74 changed files in this pull request and generated no new comments.

Suppressed comments (8)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:269

  • azd-code-reviewer: A missing dataset is represented as a 404 by this API (the client already handles that case in latestRegisteredVersion), so this branch still makes versions list <unknown> fail even though the command and its new live test promise an empty successful list. Treat only non-404 errors as failures; the renderer already normalizes a nil list to [].
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:503

  • The repository's Go 1.26 guidance requires errors.AsType rather than the legacy target-pointer errors.As pattern. Using AsType here also removes the mutable target variables and keeps this code compatible with the enforced go fix modernization.
    cli/azd/extensions/azure.ai.dataset/internal/cmd/asset_name_parity_test.go:20
  • azd-code-reviewer: Enforcing the create-time character set on every named verb makes existing datasets unmanageable if they were created by another client or an older API with a name outside this regex. The linked evaluations work explicitly calls out datasets containing spaces, and the API client already safely path-escapes such names. Restrict this validation to operations that create a new name; lookups, updates, version listings, and deletes should accept existing service-returned names.
// Every verb that takes a <name> refuses an invalid one locally.
//
// The sibling extension carries a second copy of these commands, and the guard
// reached its dataset verbs but not its evaluator ones. Walking the tree rather
// than listing the verbs is the point: a verb added later is covered without
// anyone remembering to add it here.

cli/azd/extensions/azure.ai.dataset/README.md:8

  • This setup command cannot currently succeed because this PR deliberately omits the registry.json entry; the TODO below confirms the extension is only reachable through a local source. Replace this with working local pack/publish/install instructions until the registry entry lands, otherwise the README's first onboarding step ends in an unresolved extension.
```console
$ azd extension install azure.ai.dataset
$ azd ai dataset --help
**cli/azd/extensions/azure.ai.dataset/README.md:22**
* `delete` requires `--version` and immediately errors without it, so this command table documents an invocation that cannot succeed. Include the required flag in the usage.

| azd ai dataset delete <name> | Delete a dataset version |

**cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:369**
* These new environment-variable reads are not fully reflected in the repository's required environment-variable reference: `FOUNDRY_PROJECT_ENDPOINT` currently says it is used only by `azure.ai.agents`, and `AZURE_AI_PROJECT_ENDPOINT` has no entry. Update `cli/azd/docs/environment-variables.md` so both fallback keys and this extension's precedence are documented.
**cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/pages.go:61**
* azd-code-reviewer: Writing this warning directly to process-global stderr bypasses Cobra's injected writer, so callers embedding the command cannot capture it and host-mediated invocations may lose it. Propagate truncation to the command layer and emit through `cmd.ErrOrStderr()` (or inject a warning writer into the client) instead of using `os.Stderr`.
**cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:45**
* azd-code-reviewer: Cleanup performs a single version-list call even though this PR documents and tests that the listing can remain empty immediately after a publish. Create-only live tests can therefore finish before their version is visible, skip the delete loop, and leave datasets in the shared Foundry project. Track the published version from the create response or poll the listing with a bounded retry before declaring cleanup complete.
</details>

From Copilot's review of Azure#9499.

startPendingUpload returns two things: a SAS saying where to write, and a blob
URI saying what to register. Only the SAS was checked, and they are different
fields of the same response -- so a response carrying one without the other
uploaded the bytes and then finalized against "/name.jsonl", leaving a blob
nothing points at and a publish that failed for a reason the message did not
name. Both are checked before anything is written now, and the two have separate
messages because they are not the same problem.

The other thread on that PR asked for errors.AsType over errors.As, on the
grounds that the lint workflow's `go fix -diff` would fail on it. It does not:
`go fix -diff ./...` is clean on both modules, including the errors.As added
since. Resolved as outdated.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 74 out of 75 changed files in this pull request and generated no new comments.

Suppressed comments (8)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:402

  • The added CLI contract says delete is idempotent, but a backend that represents an absent version as 404 is converted into a nonzero exit here. Treat IsNotFound as a successful no-op and only return other delete failures.
			if err := ec.datasetClient.DeleteDatasetVersion(
				ctx, name, version, ProjectEndpointAPIVersion,
			); err != nil {
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)
				}
				return messages.DeletingDatasetVersion(name, version, err)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:269

  • ListDatasetVersions can signal an unknown dataset with a 404 (the presence path already treats that response as absence), but this branch turns every error into a failed command. That contradicts the new empty-success test and means versions list <unknown> still exits nonzero on that service response. Render an empty list for IsNotFound and preserve other errors.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/README.md:22

  • This documented invocation always fails because the command requires --version. Include the required flag so readers can successfully delete a version.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:51

  • Cleanup silently ignores listing, JSON-decoding, and delete failures, so the live suite can pass while leaving dataset versions in the shared Foundry project. Make cleanup failures visible and require each deletion to succeed.
    cli/azd/extensions/azure.ai.dataset/README.md:6
  • The onboarding starts with an install command that cannot currently resolve: the same README states that no registry entry exists yet. Either add the registry entry before publishing this instruction or replace it with the local pack/publish flow so the first example is executable.
$ azd extension install azure.ai.dataset

cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:34

  • Every non-JSON value falls through to human table output, and these commands never register azdext.FlagOptions for the inherited output flag. Consequently typos such as -o yaml succeed and emit a different format instead of being rejected. Register the supported values and default on each output-producing command, as the extension SDK expects.
	return strings.ToLower(v)
}

// isJSON reports whether the command should emit machine-readable output.
func isJSON(cmd *cobra.Command) bool {
	return outputFormat(cmd) == outputJSON

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:600

  • A malformed or truncated 200 XML response is silently converted into an empty blob page. The caller then reports that the dataset has no downloadable file, hiding a service/proxy response failure as valid emptiness. Propagate the XML unmarshal error through ReadingListResponse instead.
    cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:330
  • Validate explicitly reports a malformed project path through its second return value, but every resolution branch discards it. A Foundry host with /foo is therefore accepted and only fails later as an unrelated API 404. Either reject that path or propagate the intended warning to the command output.

--version reached the incrementing path, so 'update --version 1.0' published 2.0, while 'version: 1.0' in configuration published 1.0. One word, two answers, decided by where it was written. It is now the version to publish in both places.

The flag is offered on create too; it was withheld only because of the old meaning. A version the service already holds is refused rather than stepped past -- the conflict walk exists for a listing that lags, which makes it right for a version the CLI derived and wrong for one an author named.

Nothing pinned the old behaviour, so nothing would have caught this. Two tests now cover the declared-version path, which every existing test had left empty.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 74 out of 75 changed files in this pull request and generated no new comments.

Suppressed comments (5)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:406

  • azd-code-reviewer: A 404 is turned into a failure here, which contradicts TestCLIDeleteIsIdempotent. The client tests establish that missing resources can surface as 404s, so idempotence currently depends on a particular backend returning 204 instead. Treat 404 as already deleted and only fail on other statuses.
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:275

  • azd-code-reviewer: versions list still propagates a first-page 404, even though the new CLI tests require an unknown dataset to succeed with an empty result. The client tests explicitly model an unknown dataset as a 404, so this path returns before renderDatasets can emit [] or the human-readable empty message. Convert only IsNotFound(err) into an empty DatasetList; keep propagating all other failures.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/store.go:17

  • azd-code-reviewer: This reads only the legacy extensions.ai-agents.project.context key. The current azure.ai.projects implementation writes extensions.ai-projects.context and removes the legacy key, so following this extension's own azd ai project set <endpoint> suggestion still leaves dataset commands unable to resolve the endpoint. Read the current key first and retain the agents key only as a legacy fallback.
    cli/azd/extensions/azure.ai.dataset/README.md:7
  • azd-code-reviewer: The PR deliberately omits the registry entry, and this README later acknowledges that azd extension install azure.ai.dataset cannot resolve yet. The opening quick start therefore fails in the repository state being merged. Replace it with working local pack/publish instructions until the registry entry lands, or defer this install snippet to that follow-up.
```console
$ azd extension install azure.ai.dataset
$ azd ai dataset --help

cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:50

  • azd-code-reviewer: Cleanup silently returns when listing fails or JSON is malformed, and delete failures are ignored. These live tests create persistent Foundry assets, so a transient auth/service error can leave datasets behind while the suite still passes. Make cleanup failures visible and retry the eventually consistent empty-list case before concluding there is nothing to remove.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 74 out of 75 changed files in this pull request and generated no new comments.

Suppressed comments (6)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:275

  • A 404 still takes the error path here, so versions list <unknown> exits non-zero instead of producing the empty result promised by the new tests and the comment below. Normalize only IsNotFound(err) to an empty DatasetList; keep other failures fatal. [azd-code-reviewer]
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:408

  • The command's stated idempotence is not enforced: if the service returns 404 for an already-absent version, this branch returns an error even though TestCLIDeleteIsIdempotent requires success. Ignore IsNotFound and only surface other delete failures. [azd-code-reviewer]
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)
				}
				return messages.DeletingDatasetVersion(name, version, err)

cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:510

  • Go 1.26 modernization in this repository requires errors.AsType; the reusable lint workflow runs go fix -diff and rejects outstanding suggestions. Replace both pointer-target errors.As calls so this new workflow can pass.
    cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:369
  • These new environment-variable reads are not reflected in the single-source reference: environment-variables.md still describes FOUNDRY_PROJECT_ENDPOINT as agents-only and has no AZURE_AI_PROJECT_ENDPOINT entry. Update that reference to include this extension and document the fallback/default behavior.
    cli/azd/extensions/azure.ai.dataset/README.md:6
  • This opening installation command cannot currently work because the registry entry is deliberately absent, as lines 67–70 acknowledge. Replace it with the local azd x pack/publish flow until the registry follow-up lands, rather than giving readers a command guaranteed to fail. [azd-code-reviewer]
$ azd extension install azure.ai.dataset

cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:29

  • Unsupported output values silently fall through to the human format, so a typo such as -o jsno exits successfully with non-JSON output. Register json and table as allowed values on every leaf command via azdext.RegisterFlagOptions, as established in azure.ai.routines/internal/cmd/routine_list.go:31 and azure.ai.skills/internal/cmd/skill_list.go:51. [azd-code-reviewer]
	v, err := cmd.Flags().GetString("output")
	if err != nil {
		return ""
	}
	return strings.ToLower(v)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 74 out of 75 changed files in this pull request and generated 2 comments.

Suppressed comments (8)

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:341

  • azd-code-reviewer: An unknown name can make the version-list request return 404, so show exits here with the full service response instead of the brief DatasetNotFound error promised by TestCLIUnknownDatasetIsBrief. Classify a first-page 404 as the missing dataset before wrapping other listing failures.
				list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
				if err != nil {
					return messages.ResolvingLatestDatasetVersion(name, err)
				}

cli/azd/extensions/azure.ai.dataset/README.md:22

  • azd-code-reviewer: The documented delete invocation omits the required --version flag, so copying it always fails with “--version is required.” Include the required version argument in the command table.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:275

  • azd-code-reviewer: This still returns an error when the service answers 404 for an unknown dataset, contradicting the new CLI contract and TestCLIVersionsListOfAnUnknownNameSucceeds. Convert that specific response to an empty list so human output and -o json reach the intended renderer.

This issue also appears on line 338 of the same file.

			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:29

  • azd-code-reviewer: No dataset command registers SDK FlagOptions for the inherited output flag, so any typo is accepted and every non-JSON value silently becomes human output (-o jsno exits 0 with a table). Register json and table as allowed values with the intended default on each output-producing command so scripts fail fast and help/metadata advertise the real formats.
	v, err := cmd.Flags().GetString("output")
	if err != nil {
		return ""
	}
	return strings.ToLower(v)

cli/azd/extensions/azure.ai.dataset/README.md:8

  • azd-code-reviewer: This is currently a guaranteed failing onboarding command because this PR deliberately omits the registry entry, as the TODO below acknowledges. Until the follow-up lands, replace it with the local pack/publish installation flow or clearly mark the registry install command as unavailable; otherwise the first instruction in the README leaves every new user stuck.

This issue also appears on line 22 of the same file.

```console
$ azd extension install azure.ai.dataset
$ azd ai dataset --help
**cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:512**
* azd-code-reviewer: This new code uses the legacy `errors.As` target pattern even though the module is on Go 1.26 and the repository’s lint workflow rejects `go fix -diff` suggestions. Use `errors.AsType` (and run `go fix ./...`) so the new lint workflow does not fail.
**cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:51**
* azd-code-reviewer: `Dataset` is used as both the service wire model and the public `-o json` model. Because these duplicate fields retain their original tags, a response using `data_uri` is re-emitted as `data_uri`, while another backend spelling becomes `dataUri`; scripts cannot rely on one schema even though `ResolvedBlobURI` normalizes human output. Emit a dedicated output DTO or implement normalized JSON marshaling with one canonical URI field.
**cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:207**
* azd-code-reviewer: `--from-file` has no size bound, but this reads the entire dataset into memory, converts it to a string during validation, and later copies it back to `[]byte` for upload. Large but valid datasets can therefore consume several times their file size and terminate the extension. Validate rows from a stream and upload from the file (or enforce a documented size limit) instead of materializing the complete dataset repeatedly.
</details>

"%s/%s/versions/%s/startPendingUpload",
pathDatasets, url.PathEscape(name), url.PathEscape(version),
)
return doRequestTyped[PendingUploadResponse](c, ctx, http.MethodPost, path, nil, json.RawMessage(`{}`), apiVersion)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed in principle, and deliberately not changed, because the evidence in this repo points two ways.

pendingUploadType is sent by two other extensions here, with different values: azure.ai.models sends TemporaryBlobReference (foundry_client.go:143, added deliberately per its changelog), while azure.ai.training sends BlobReference and comments it as "Always". Our request model already carries the field; only the value is missing.

The current {} is not theoretical tolerance -- it is the path every dataset in the last two bug bash builds was uploaded through, so the service is choosing a default today. Sending a guessed enum is the one change here that can turn a working call into a 400, and I have no way to tell which of the two values this API version expects: there is no datasets spec in the specs repo I can reach, and the response echo (pendingUploadType on PendingUploadResponse) only reports what the server picked, which I would need a live capture to read.

So the safe order is: capture what the service echoes on a real upload, then send exactly that. Tracked rather than guessed.

Comment on lines +338 to +340
DataURI: dataURI,
}
return doRequestTyped[Dataset](c, ctx, http.MethodPut, path, nil, request, apiVersion)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same disposition, and for the same reason: this is the path every dataset in the last two bug bash builds was registered through, so I am not switching its verb and content type on an unverified contract description.

To be clear about what I could and could not confirm. I could not verify it -- the specs repo I have access to carries no datasets spec, so I cannot check whether Datasets_CreateOrUpdateVersion is PATCH/application/merge-patch+json at 2025-11-15-preview, nor whether name, version and isReference are rejected as read-only rather than ignored. What I can say is that the current PUT/application/json with {name, version, type, dataUri} works against the live service today.

Your underlying point stands on its own though: sending fields the caller does not own is a latent break, and it costs nothing to stop sending them once the shape is confirmed. If a backend does tighten this, the failure is every upload, which is worth getting ahead of rather than discovering.

Tracked so it is verified against the spec and a live call before being changed, rather than swapped on this PR and found in a bug bash.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 75 out of 76 changed files in this pull request and generated 1 comment.

Suppressed comments (10)

Previously missed (7) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:275

  • azd-code-reviewer: A 404 from the versions endpoint still exits with an error, even though this command's stated contract and live test require an unknown name to return an empty successful list. Convert only IsNotFound into an empty DatasetList; continue propagating all other failures.

This issue also appears on line 338 of the same file.

			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:409

  • azd-code-reviewer: Returning an error for a missing version contradicts the idempotent-delete contract pinned by TestCLIDeleteIsIdempotent. Treat a 404 as successful no-op deletion while preserving other service failures.
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)

cli/azd/extensions/azure.ai.dataset/README.md:6

  • azd-code-reviewer: This installation command cannot currently resolve because the same README states that registry.json deliberately has no azure.ai.dataset entry. Replace it with a working local pack/publish workflow until the registry entry lands, or add this snippet only with that follow-up.

This issue also appears on line 22 of the same file.

$ azd extension install azure.ai.dataset

cli/azd/extensions/azure.ai.dataset/CHANGELOG.md:18

  • azd-code-reviewer: No command calls DownloadDatasetContent; show renders metadata only. This first-release changelog therefore advertises content readback that users cannot invoke. Remove this bullet or expose the behavior through a documented command.
- Reads dataset content back, whether the service hands out a blob URI or the
  container holding it.

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:51

  • azd-code-reviewer: These wire aliases leak into the public -o json schema because commands encode Dataset directly. A snake_case service response emits data_uri, while a camelCase response emits dataUri, so scripts get different field names for the same command. Map API models to a stable output DTO using ResolvedBlobURI() before rendering lists and details.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:207
  • azd-code-reviewer: Upload currently buffers the entire dataset several times: ReadFile allocates the file, validation converts rows to strings, the function returns another string, and UploadVersion converts it back to []byte. Large Foundry datasets can therefore exhaust memory before the upload starts. Validate from a stream, rewind/reopen the file, and pass an io.Reader to the blob request instead of materializing the full payload.
    cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:457
  • azd-code-reviewer: This branch explicitly covers transient token-helper failures where the login may still be valid, but it records all of them as login_expired. That misclassifies extension error telemetry and can overstate expired-login failures. Use the generic auth-failure code unless the error provides specific evidence that the login expired.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:340

  • azd-code-reviewer: When the initial versions lookup returns 404, show exposes the service response instead of the brief DatasetNotFound error required by TestCLIUnknownDatasetIsBrief. Handle a not-found listing the same way as an empty listing.
				list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
				if err != nil {
					return messages.ResolvingLatestDatasetVersion(name, err)

cli/azd/extensions/azure.ai.dataset/README.md:22

  • azd-code-reviewer: delete requires --version, so the documented command always fails with “--version is required.” Include the required flag in the command table.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:510

  • azd-code-reviewer: This is the only remaining pre-Go-1.26 errors.As pattern in the extension. The repository requires errors.AsType, which avoids mutable target variables and is already used elsewhere in this module.

Engineering tracking, not product documentation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 74 out of 75 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/pages.go:107

  • azd-code-reviewer: Relative continuation links are resolved against the project endpoint rather than the URL that produced the page. For a real endpoint ending in /api/projects/<project>, a query-only link such as ?page=2 drops /datasets, and the tested /datasets?page=2 form drops the entire project path. Resolve each continuation against the current page URL and cover the test with a project-path endpoint.
    cli/azd/extensions/azure.ai.dataset/internal/cmd/root.go:63
  • azd-code-reviewer: None of these executable commands registers the inherited --output contract with azdext.RegisterFlagOptions. As a result, unsupported values such as -o yaml silently fall through to table output, and generated metadata/completions do not advertise json or table. Register those allowed values with a table default on every leaf command, including versions list.
		newDatasetCreateCommand(),
		newDatasetUpdateCommand(),
		newDatasetListCommand(),
		newDatasetShowCommand(),
		newDatasetDeleteCommand(),

cli/azd/extensions/azure.ai.dataset/README.md:22

  • The command table omits the required --version flag, so copying this documented command immediately fails with “--version is required.” Include the flag in the command syntax.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:50

  • azd-code-reviewer: Cleanup silently ignores both deletion failures here and listing/JSON failures above, so the live suite can pass while leaving paid project assets behind. Make cleanup report failures and account for the service's eventually consistent version listing before declaring cleanup complete.

cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:370

  • These new reads are not reflected in the canonical environment-variable reference: FOUNDRY_PROJECT_ENDPOINT is still described as agents-only, and AZURE_AI_PROJECT_ENDPOINT has no entry. Update cli/azd/docs/environment-variables.md so both variables document this extension and the fallback behavior.
    cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:510
  • This module targets Go 1.26, and the repository's go fix -diff CI check requires the modern errors.AsType form. Replace these target variables and errors.As calls so the new lint workflow does not report a modernization diff.

Comment thread cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go Outdated
…ables

The delete removed the remote version the moment it was invoked, with no question and no way to say yes in advance. It now follows the same contract as the other Foundry delete commands. The environment-variable reference gains sections for both extensions.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 76 out of 77 changed files in this pull request and generated 3 comments.

Suppressed comments (6)

Previously missed (5) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:277

  • azd-code-reviewer: An unknown dataset can return 404 from ListDatasetVersions (the API tests explicitly model that), so this branch exits before the documented empty-list rendering. TestCLIVersionsListOfAnUnknownNameSucceeds will therefore fail against that service behavior. Convert only a not-found response to an empty DatasetList and keep propagating other errors.

This issue also appears on line 414 of the same file.

			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:45

  • azd-code-reviewer: These wire aliases leak into the public -o json schema because commands serialize Dataset directly. A camel-case response emits dataUri, while a snake-case response emits data_uri (and a response containing both emits both), so scripts do not get the stable shape promised by the CLI. Keep the service aliases in the transport model, but map command output to one normalized DTO using ResolvedBlobURI().
    cli/azd/extensions/azure.ai.dataset/README.md:22
  • This documented command always fails because --version is required by newDatasetDeleteCommand. Include the required flag so the command table is executable as written.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/validator.go:41

  • azd-code-reviewer: The non-fatal path warning is discarded by every Resolve call site, so an account URL such as https://acct.services.ai.azure.com is accepted and later requests the wrong /datasets route instead of reporting the missing /api/projects/<project> path. Either reject this shape here or surface the warning through the command layer; returning a boolean that nobody reads leaves the endpoint validation incomplete.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/pages.go:61
  • azd-code-reviewer: This client-layer write bypasses Cobra's injected output/error writers. The warning cannot be captured by callers or tests and appears as process-global terminal output even during machine-readable commands. Return truncation information to the command layer (or inject a writer) and emit it through cmd.ErrOrStderr().

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:418

  • azd-code-reviewer: This turns a 404 into an error, contradicting the new idempotent-delete contract and TestCLIDeleteIsIdempotent. Treat not-found as an already-deleted success while continuing to report every other service failure.
			if err := ec.datasetClient.DeleteDatasetVersion(
				ctx, name, version, ProjectEndpointAPIVersion,
			); err != nil {
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)

Comment thread cli/azd/docs/environment-variables.md Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go Outdated
Comment thread cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go
…tions

Delete now asks before removing published data, so the live tests and their cleanup pass --force; cleanup reports failures instead of leaking datasets into a shared project. AZURE_AI_PROJECT_ENDPOINT is read from the azd environment as well as the host shell, which the reference did not say.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 76 out of 77 changed files in this pull request and generated 3 comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:173

  • azd-code-reviewer: None of the dataset leaf commands register allowed values for the inherited --output flag. NewExtensionRootCommand only validates output through RegisterFlagOptions (cli/azd/pkg/azdext/extension_command.go:179-182), so values such as -o yaml or -o none silently produce human output. Register table and json on every leaf, as done in azure.ai.connections/internal/cmd/connection.go:97-99, and test invalid-format rejection.
	cmd.Flags().StringVar(&version, "version", "",
		"Version to publish. Omit to publish the next version after the latest registered.")
	cmd.Flags().StringVar(&endpointFlg, "project-endpoint", "", "Foundry project endpoint.")
	return cmd

cli/azd/extensions/azure.ai.dataset/README.md:22

  • azd-code-reviewer: This documented invocation always fails because delete requires --version (internal/cmd/dataset.go:399-400). Include the required flag so the command table provides usable syntax.
| `azd ai dataset delete <name>` | Delete a dataset version |

Comment thread cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go Outdated
Comment thread cli/azd/docs/environment-variables.md Outdated
Four findings from the review, all the same shape: a command reporting
success for something it did not actually do.

Answering "no" to a delete exited 1. It came back as an ordinary error,
which azdext reports and exits on, so a reader who deliberately declined
got the same exit code as one whose delete broke -- and every script that
checks reads that as a failure. confirmDelete now returns whether to go
ahead separately from whether anything went wrong, in both extensions.
The refusal under --no-prompt still errors, because there the command
could not do what it was asked; declining is the command doing exactly
what it was asked.

A page walk that hits a repeated nextLink, or the page cap, now fails
instead of handing back the pages that arrived. Both walkers said in
their own doc comments that a short listing is "a silent wrong answer
rather than a short one", and then broke out of the loop and returned
nil. Those rows choose the latest version and decide whether a name is
ambiguous, so the caller was not getting less of the truth, it was
getting a different answer. All four page-walk tests encoded the old
contract and now encode this one.

A configuration lock that could not be taken warned and continued
unlocked. That is exactly the lost update the lock exists to prevent: the
holders read the configuration, change one entry and write the whole
document back, so two of them both report success and the later write
drops the earlier one's entry. It now refuses, with the directory named
so a reader knows which configuration is busy and that another process is
why. The timeout became a var solely so the test does not wait 30 seconds
to prove it.

The README quickstart said `azd ai eval run`, which prints help, matching
the manifest entry fixed in the previous commit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 76 out of 77 changed files in this pull request and generated no new comments.

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:277

  • azd-code-reviewer: An unknown dataset can return 404 from ListDatasetVersions (as TestAFirstPage404IsStillAbsence establishes), so this returns an error before reaching the empty-list behavior promised below and exercised by the CLI test. Translate a first-page not-found into an empty DatasetList; continue surfacing every other error.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:340

  • azd-code-reviewer: This resolves “latest” from one versions-list request, but the client documents that this listing is eventually consistent after publishing. Immediately after create it can therefore report the dataset missing, and after update it can show the previous version. Use a consistent latest-version lookup or bounded verification/retry before selecting the result.
				list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)

cli/azd/extensions/azure.ai.dataset/README.md:6

  • azd-code-reviewer: This onboarding command cannot work with this PR: the registry entry is intentionally omitted, and this README itself says the extension cannot be resolved until that entry exists. Replace it with a verified local-source installation flow for now, or defer this example until the registry follow-up lands.
$ azd extension install azure.ai.dataset

cli/azd/extensions/azure.ai.dataset/README.md:22

  • azd-code-reviewer: The documented delete invocation always fails because the implementation requires --version. Include the required flag so readers can run the command as shown.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:262

  • azd-code-reviewer: Uploading loads the entire dataset into memory, converts it to a string, and then allocates another full []byte copy here before the PUT. With no documented size limit, a large dataset can consume several times its file size and terminate the CLI. Validate and upload from a stream, or enforce and document a bounded maximum size.

cli/azd/docs/environment-variables.md:172

  • azd-code-reviewer: For this extension, AZD_EXT_DEBUG is parsed by isDebug to enable SDK/standard-library logging; the custom-command entry point never calls WaitForDebugger. This description therefore promises a debugger prompt that does not occur and should describe debug logging instead.
| `AZD_EXT_DEBUG` | If true, waits for a debugger to attach before the command runs. |

cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:535

  • azd-code-reviewer: The repository's Go 1.26 guidance requires errors.AsType instead of the legacy target-pointer form of errors.As; the pre-commit go fix check can rewrite this. Use typed checks for both credential error types.

Three findings, all of the same kind: a message that sends the reader
somewhere useless.

A Foundry endpoint in Azure Government or China was reported as "not a
recognized Foundry host". The host is recognized -- it is correct, and the
extension simply cannot reach that cloud, because the token it asks for is
scoped to a public-cloud audience. A reader with a working Government
project would go and check a URL that was already right. Those hosts are
now named, and the refusal says what it is. Deliberately not "fixed" by
adding the suffix to the accepted list: the audience would still be wrong
and the failure would move to a 401 with nothing to act on.

AZD_EXT_DEBUG was documented as waiting for a debugger to attach. It does
not; it turns on the same diagnostic logging as --debug, into a private
temporary file. Documented for both extensions.

The live export test asserted the CSV header "run_id,status,criterion,..."
while the exporter writes "testing_criteria" -- as the assertion twenty
lines above it already checked. That subtest could not have passed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 77 out of 78 changed files in this pull request and generated no new comments.

Suppressed comments (8)

Previously missed (8) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:277

  • azd-code-reviewer: The comment and CLI test define an unknown name as an empty successful listing, but a backend that returns 404 takes this unconditional error path. Normalize IsNotFound(err) to an empty DatasetList; otherwise versions list <unknown> still exits 1 on that valid service response.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:426

  • azd-code-reviewer: TestCLIDeleteIsIdempotent says deleting an unregistered version succeeds, but a service/backend that expresses absence as 404 returns an error here. Treat not-found as successful deletion so idempotence does not depend on whether a deployment returns 204 or 404 for the same state.
			if err := ec.datasetClient.DeleteDatasetVersion(
				ctx, name, version, ProjectEndpointAPIVersion,
			); err != nil {
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/list.go:80

  • azd-code-reviewer: This fallback orders only the final digit run, so accepted versions such as 1.10.0 and 1.9.9 compare as 0 and 9. show without --version can therefore return the older version, and the next automatic upload can advance from the wrong baseline. Either compare numeric components or reject explicit versions outside the supported decimal/trailing-counter grammar.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:510
  • azd-code-reviewer: A repeated marker proves the blob listing is incomplete, but this treats it as a normal final page and returns partial names. DownloadDatasetContent can then choose the wrong fallback file or report that no file exists. Return a truncation error on a repeated marker, as followPages already does for repeated nextLink values.
    cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:402
  • azd-code-reviewer: Every direct-download error triggers the container fallback, although only the expected 409 indicates that a file-looking URI is actually a container. A 403, timeout, or cancellation is instead masked by a second listing request and an unrelated final error. Preserve the storage status and fall back only for the container response; return other failures directly.
    cli/azd/extensions/azure.ai.dataset/README.md:22
  • The documented delete invocation always fails because --version is required by the command. Include the required flag in the command table so users can run the example successfully.
| `azd ai dataset delete <name>` | Delete a dataset version |

cli/azd/extensions/azure.ai.dataset/README.md:6

  • This quick-start command cannot resolve because this PR deliberately omits the registry entry, as the TODO at lines 67–70 notes. Mark it as a post-release command or replace it with complete local pack/publish/install steps so the README's first workflow is currently usable.
$ azd extension install azure.ai.dataset

cli/azd/extensions/azure.ai.dataset/CHANGELOG.md:3

  • This extension is still unpublished—the README says the registry entry remains TODO—so an official release dated 2026-08-20 has not occurred. Keep the entry marked Unreleased until the release pipeline publishes it; otherwise the release history records a date before the artifact was available.
## 1.0.0-beta.17 (2026-08-20)

`ListContainerBlobs` ended its walk two ways that both looked like
success: a NextMarker that repeats, and running out of the page cap. Both
returned the names collected so far with a nil error.

That one matters more than the two already fixed. These names are what
`pickDatasetBlob` chooses the `.jsonl` from, so a container whose data
file sits on a page the walk never reached does not produce a shorter
list -- it produces the wrong file, or "no dataset file" about a dataset
that plainly has one, with nothing in the output to say why.

An empty marker is still the clean end and still returns normally. The
other two now return pageWalkError{ListingTruncated}, matching what the
cursor and nextLink walkers in the same package already do.

Both existing tests asserted the old contract and now assert this one, and
a page-cap case was added, in both extensions. Mutation-tested: putting
`|| next == marker` back on the clean-end branch fails
TestListContainerBlobsRefusesARepeatedMarker in both.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 77 out of 78 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

Previously missed (4) — in code that hasn't changed since the last review.

cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/store.go:17

  • azd-code-reviewer: This reads only the legacy agents key, but azd ai project set writes extensions.ai-projects.context and removes the legacy key during migration (azure.ai.projects/internal/cmd/project_context_store.go:14-20,84-117). As a result, the resolver's own suggested azd ai project set remediation still leaves this extension without an endpoint. Read the projects key first and retain this key only as a legacy fallback; update the documented cascade too.
    cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:277
  • azd-code-reviewer: The comment below promises that an unknown name produces an empty successful list, but a backend 404 is returned here before rendering. The client tests explicitly model 404 as the service's unknown-dataset response, so normalize that status to an empty list and preserve errors for other statuses.
			list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
			if err != nil {
				return messages.ListingDatasetVersions(name, err)
			}

cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:207

  • azd-code-reviewer: ReadFile loads the entire dataset, the return conversion retains another full string copy, and UploadVersion converts it back to []byte before sending. With no documented size cap, ordinary large datasets can consume several times their size in memory and terminate the CLI. Validate the JSONL as a stream, then reopen the file and stream it into the upload request.
    cli/azd/extensions/azure.ai.dataset/CHANGELOG.md:18
  • azd-code-reviewer: This release note promises that users can read dataset content back, but DownloadDatasetContent has no production caller and dataset show only emits metadata/URI fields. Remove this claim or expose a command/flag that actually writes the content.
- Reads dataset content back, whether the service hands out a blob URI or the
  container holding it.

cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:425

  • azd-code-reviewer: The live test documents delete as idempotent, but a service 404 is converted into a command failure here. Treat not-found as already deleted and continue to the normal success response; only propagate other delete failures.
			if err := ec.datasetClient.DeleteDatasetVersion(
				ctx, name, version, ProjectEndpointAPIVersion,
			); err != nil {
				if dataset_api.IsNotFound(err) {
					return messages.DatasetVersionNotFound(name, version)
				}
				return messages.DeletingDatasetVersion(name, version, err)

// NextVersion(""), so probing a hardcoded "1" never found a dataset this CLI
// had just created -- which is the one case the probe exists for. "1" is still
// probed because a generation job, the SDK or the portal can register one.
var firstDatasetVersions = []string{dataset_api.NextVersion(""), "1"}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/evals Evaluations, benchmarks, quality checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add an azd ai dataset extension for Foundry datasets

2 participants